Include Settable Properties in Default Constructor#445
Conversation
|
Now that I see those failures, I'm pretty sure I tried doing this and that's exactly why I gave up 😆 Maybe we could somehow use the property default to determine if it gets turned into a constructor argument? |
|
Tests pass, but one of the examples in # These can be useful if you want to deprecate a property
person <- new_class("person", properties = list(
first_name = class_character,
firstName = new_property(
getter = function(self) {
warning("@firstName is deprecated; please use @first_name instead", call. = FALSE)
self@first_name
},
setter = function(self, value) {
warning("@firstName is deprecated; please use @first_name instead", call. = FALSE)
self@first_name <- value
self
}
)
))
hadley <- person(first_name = "Hadley")
#> Error: <person>@first_name must be <character>, not <NULL>
#> In addition: Warning message:
#> @firstName is deprecated; please use @first_name instead Some alternatives for how to omit a property with a custom
I think requiring option 1, a custom constructor here, is a reasonable solution. |
|
The deprecated wrapper is such a nice idea and I'd really prefer not to have require a custom constructor for it. How do defaults work with settable properties currently? |
|
We take whatever value is supplied to |
|
If we add a Foo <- new_class("Foo", properties = list(
x = new_property(default = required()),
y = new_property(default = omit())
))This would result in: formals(Foo) == alist(x =) |
|
Currently, the example doesn’t quite work. Adding a Some other approaches: 1. Infer Initialization ContextWe could modify the firstName = new_property(
class = NULL | class_character,
setter = function(self, value) {
if (is.null(value)) return()
warning("@firstName is deprecated; please use @first_name instead")
self@first_name <- value
self
}
)This suggests a potential need for a general mechanism to detect when the object is being initialized: setter = function(self, value) {
if (initializing(self)) {
# Handle property during initialization
} else {
# Handle property after initialization
}
}2. Ignore Missing Values in
|
|
Thanks for exploring this! Your final idea sounds reasonable to me; I presume you'd start with a PR to make this the default for regular properties first? |
|
Thanks tackling this. I agree that the second approach is best. One minor thing about the example is that |
|
How about this, starting from the current example in Person <- new_class("Person", properties = list(
<...unchanged...>
default = quote(...)
))Produces: The |
R/constructor.R
Outdated
| ) | ||
|
|
||
| is_dots <- vlapply(self_args, identical, quote(...)) | ||
| if (any(is_dots)) { |
There was a problem hiding this comment.
Maybe if ... is present it should always be the last argument? Or the first? Otherwise you end up with a mix of arguments that can/can't be supplied by position.
(I would also consider the idea that the first argument to every constructor should be ... just to force all arguments to be fully named)
There was a problem hiding this comment.
the first argument to every constructor should be
...
I think this is going to be too annoying in practice. Many classes will only have a small handful of properties, and being unable to positionally match 1, 2, or 3 would be cumbersome.
Otherwise you end up with a mix of arguments that can/can't be supplied by position.
I think #446 might solve this by allowing the user to specify a property that is unset by default but still present as a named arg in the constructor formals that can match positionally.
There was a problem hiding this comment.
In that case, I think you should make ... the last argument, if it's present.
There was a problem hiding this comment.
I updated the code so ... is added at the end of the constructor. Note, deprecating this way now will potentially change the position of later arguments E.g.,
# before deprecating:
function(firstName, lastName, country)
# after deprecating `firstName` and `lastName`:
function(name, country, ...)
# whereas with the previous approach, this was possible:
function(name, ..., country)There was a problem hiding this comment.
Maybe we should chat a bit more about this live, because I don't quite see the problem.
Previously, the generated constructor had: new_object(, ... = ...) It now has: new_object(, ...)
|
I've revised the PR to take a conservative approach, supporting the different use cases without introducing any new features that would warrant further discussion (e.g., the meaning of a missing or I updated the examples to show how to achieve all the different use cases in #449:
In the future, we may add features that would make some of these examples more ergonomic, but I don't think we'd introduce any changes that would break them as they are currently written. This PR is ready for another review and merge. |
hadley
left a comment
There was a problem hiding this comment.
Looks great to me. Thanks for all your work on this!
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
| ```{r} | ||
| Person <- new_class("Person", properties = list( | ||
| first_name = class_character, | ||
| firstName = new_property( |
There was a problem hiding this comment.
Shouldn't this firstName property be of class_character? Even though it is deprecated, we still want it to function properly. And I guess we would really want it to be a scalar. What happens if there is no reasonable default, as in this case? Could the default be made to be quote(first_name)? Then the setter avoids a warning if the value is the same as first_name.
There was a problem hiding this comment.
Thanks! I updated the example to show class = class_character, default = quote(first_name). The check in the setter is now identical(value, self@first_name).
Another approach would be to make the class NULL | character, and keep the simple is.null() check in the setter.
|
I like the simplicity of this approach. Please just see the comment I made on the example. |
…://github.com/RConsortium/S7 into include-dynamic-settable-props-in-constructor
This patch implements the idea proposed in this comment and discussed in the last working group (WG) meeting: summarized here.
With this change, if a property has a
setterfunction, the property will be included as an argument in the default class constructor, and the constructor will call the custom setter for that property.